fix(fs): Escape paths in Scope methods #2070
Conversation
Package Changes Through cc182fbThere are 15 changes which include upload with minor, upload-js with minor, deep-link with patch, deep-link-js with patch, fs with minor, persisted-scope with minor, log-plugin with patch, log-js with patch, fs-js with patch, localhost with minor, opener with major, opener-js with major, positioner-js with minor, positioner with minor, sql with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
|
this is because the scope module is used in build.rs, see plugins-workspace/plugins/fs/build.rs Lines 10 to 12 in b42064d you can just add |
|
Ahhhhhh yes, forgot about that... |
|
Oh and thanks. I'll fix it when I'm back |
|
this seems to work for now but i'm less than happy. i think it may make sense to release this is a temp fix/workaround and wait for lucas to get back to discuss further steps. Alternatively i'm thinking about storing a idk, i guess i'll take another look tomorrow. |
actually I think we maybe able to reuse the one from |
|
i already miss the time when it was ok to break user facing apis 😂 what do you think about the new approach? |
|
also thought about a new FsExt where fs_scope() returns a new scope based on tauri's fs::Scope and maybe making the old apis no-op but idk, everything i can think of is ugly |
| } | ||
| } | ||
|
|
||
| fn is_forbidden<P: AsRef<Path>>( |
There was a problem hiding this comment.
why is this implemented separately here?
There was a problem hiding this comment.
i can implement it in tauri::fs::Scope too and re-use that but wanted to keep the plugin compatible with all tauri v2 versions for now.
not too happy about the approach itself but merging instances of tauri::fs::Scopes isn't possible rn so this was the next best thing to make the draft work.
There was a problem hiding this comment.
let's ship with this for now, but I think we should still add it in tauri and then migrate the fs plugin later to that.
There was a problem hiding this comment.
ok, will open a pr later if i don't forget
|
this should be done now apart from the (ongoing?) discussion about is_forbidden |
| } | ||
| } | ||
|
|
||
| fn is_forbidden<P: AsRef<Path>>( |
There was a problem hiding this comment.
let's ship with this for now, but I think we should still add it in tauri and then migrate the fs plugin later to that.
| if is_forbidden(&fs_scope.scope, &path, require_literal_leading_dot) | ||
| || is_forbidden(&scope, &path, require_literal_leading_dot) | ||
| { | ||
| return Err(CommandError::Plugin(Error::PathForbidden(path))); | ||
| } | ||
|
|
||
| if fs_scope.scope.is_allowed(&path) || scope.is_allowed(&path) { |
There was a problem hiding this comment.
do really need to manually call is_forbidden, doesn't scope.is_allowed check for forbidden/denied patterns?
There was a problem hiding this comment.
the problem is that is_allowed returns false for paths that are explictly denied and paths that are neither allowed nor denied.
When we're checking for 2 scopes at the same time we only care about the explicitly denied paths because those take precedence of explictly allowed paths.
A path that's neither allowed nor denied in the first scope could still be explicitly allowed by the second scope.
Alternatively a way to create a new Scope from 2 existing scopes would be cool too, then is_allowed would work for us here.
There was a problem hiding this comment.
I guess we can improve this a bit by adding the Scope::is_forbidden in tauri
opening as a draft because it doesn't compile locally, complaining about glob not being a dependency but only if it's uses in scope.rs - if it fails in ci too i'll just move it to lib.rs.
if anyone spots my mistake here, please enlighten me...
fixes tauri-apps/tauri#11707
fixes tauri-apps/tauri#11708